Blocks: Improve UI for all Inspector Controls #5995

gziolo · 2018-04-05T05:40:27Z

Description

Follows up #5952.

I just noticed a possible improvement, that in my option does not block the PR. When the block has advanced controls but hasn't normal controls we show two bars:
.
The fix for this is not easy because even when normal inspector controls and advanced ones are empty, we render empty container divs, so using nth-child or first/last is not possible. We can either remove this container divs if possible. Or add classes to them with empty/not empty state and we could add the border when advanced controls with content follow normal controls with content (using + selector).

This PR also improves the default view of the Custom Size slider:

Before:

After:

How Has This Been Tested?

Manually.

Checking the diff of this PR is much easier without whitespace changes: https://github.com/WordPress/gutenberg/pull/5995/files?w=1.

Screenshots (jpeg or gifs if applicable):

Paragraph

Latest Posts

Shortcode

List

Types of changes

UI improvements.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code has proper inline documentation.

jasmussen · 2018-04-05T06:34:33Z

Beauuuutiful. So many little things that just improve the whole picture. Thank you.

To me this looks completely shippable as is, experience wise. Code looks good too, though I see a failing test.

gziolo · 2018-04-05T07:02:39Z

I think I might introduced some regression when uodating RangeControl. I will investigate

gziolo · 2018-04-05T08:04:36Z

It was a different issue. In line https://github.com/WordPress/gutenberg/pull/5995/files#diff-9e70015597c35b4faecd9a6beae81344R245 fontSize was applied also for the default font size which isn't necessary.

gziolo · 2018-04-05T11:22:40Z

Let's get this UI fixes for the 2.6 release.

paulwilde · 2018-04-05T12:50:17Z

Although I like the changes, this has had an impact on my implementation where I've been adding additional controls to core blocks.

As an example with the 'Advanced' Settings disabled (Custom CSS and Anchors), I have added 3 additional controls to the Heading Settings. The last 3 at the bottom:

However, when the panel is closed, these additional settings are no longer contained within the same panel:

I don't want to be limited to where I need to create a separate expandable panel for very basic additional settings such as text colour as it adds more weight to the UI. I'd want them to live in the same main 'Heading Settings' panel.

As there is currently no documentation on how controls can be added to existing blocks I used the implementation that the Custom CSS and Anchors uses in order to learn on how to add these additional controls. Maybe its possible to target the controls to be rendered at the bottom of the 'Heading Text' component, although I couldn't figure it out.

For clarify sake, here is what I'm doing to add new controls:

function onUpdate( BlockEdit ) {
    const WrappedBlockEdit = ( props ) => {
        if ( 'core/heading' !== props.name ) {
            return <BlockEdit { ...props } />
        }

        const { attributes, setAttributes, isSelected } = props
        
        return [
            <BlockEdit { ...props } />,
            isSelected && <InspectorControls key="inspector">
                <ToggleControl
                    label={ __( 'Custom Control' ) }
                    checked={ !! attributes.customControl }
                    onChange={ onChangeEvent }
                />
            </InspectorControls>,
        ]
    }
    return WrappedBlockEdit
}
addFilter( 'blocks.BlockEdit', 'custom/heading/update', onUpdate )

jorgefilipecosta · 2018-04-05T13:02:47Z

blocks/library/paragraph/index.js

@@ -104,6 +104,8 @@ class ParagraphBlock extends Component {
 		if ( customFontSize ) {
 			return customFontSize;
 		}
+
+		return FONT_SIZES.regular;


We are setting the default size for font Size as regular making the range control start in some value. But we don't apply the regular class.
This may cause problems, imagine for example I set a CSS rule for paragraphs nested inside the cover image to have size 20px. These changes make the default size for paragraph appear as 16px but we don't set the class so the size will be in fact the one set with CSS for paragraphs inside cover image 20px.
Essentially I think there is a difference of state between not having size specified, no class et all where we use the default css rules depending on the context, and having regular font size where we use the regular class.

This may cause problems, imagine for example I set a CSS rule for paragraphs nested inside the cover image the have size 20px.

Yes, I didn't think about that. Good catch. If you use fontSize or customFontSize prop it won't be an issue. Updating CSS is tricky. I got your point though.

Any idea how to move the slider to the proper position without updating the state then? :)

At the moment it's a very bad user experience when you start updating the font size and it gets much bigger than it initially was.

I agree the solution we had is not perfect, I think a possible improvement maybe use getComputedStyles to try to understand the actual size of font, and position the initial range slider position in that value, but without setting the value itself (we would have an initial position prop in the slider to set it to initial value when no value exists) the value would only be set if the user changed it. So we would still have a difference between the state user explictly set some value or no value was chosen.

I opened PR to revert this change. This is more complex than I expected and not that important to spend a significant amount of time on it. See: #6075.

gziolo added [Feature] Blocks Overall functionality of blocks Needs Design Feedback Needs general design feedback. labels Apr 5, 2018

gziolo added this to the 2.6 milestone Apr 5, 2018

gziolo self-assigned this Apr 5, 2018

gziolo requested review from karmatosed, jasmussen and jorgefilipecosta April 5, 2018 05:40

gziolo force-pushed the update/inspector-controls-ui branch from bb2f4b4 to 96abec8 Compare April 5, 2018 08:00

gziolo added 2 commits April 5, 2018 13:08

Blcoks: Improve UI for all Inspector Controls

dcad1f0

Blocks: Fix custom size slider for Paragraph block

9a61a2a

gziolo force-pushed the update/inspector-controls-ui branch from 96abec8 to 9a61a2a Compare April 5, 2018 11:14

gziolo merged commit e7efe5b into master Apr 5, 2018

gziolo deleted the update/inspector-controls-ui branch April 5, 2018 11:22

jorgefilipecosta reviewed Apr 5, 2018

View reviewed changes

gziolo removed the Needs Design Feedback Needs general design feedback. label Apr 9, 2018

This was referenced Apr 9, 2018

Blocks: Revert the default fonti size changes for Paragraph #6075

Merged

Allow for core block Inspector Controls to be extended and removed #6023

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blocks: Improve UI for all Inspector Controls #5995

Blocks: Improve UI for all Inspector Controls #5995

gziolo commented Apr 5, 2018 •

edited

Loading

jasmussen commented Apr 5, 2018

gziolo commented Apr 5, 2018 •

edited

Loading

gziolo commented Apr 5, 2018

gziolo commented Apr 5, 2018

paulwilde commented Apr 5, 2018 •

edited

Loading

jorgefilipecosta Apr 5, 2018 •

edited

Loading

gziolo Apr 5, 2018 •

edited

Loading

jorgefilipecosta Apr 5, 2018 •

edited

Loading

gziolo Apr 9, 2018

Blocks: Improve UI for all Inspector Controls #5995

Blocks: Improve UI for all Inspector Controls #5995

Conversation

gziolo commented Apr 5, 2018 • edited Loading

Description

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

jasmussen commented Apr 5, 2018

gziolo commented Apr 5, 2018 • edited Loading

gziolo commented Apr 5, 2018

gziolo commented Apr 5, 2018

paulwilde commented Apr 5, 2018 • edited Loading

jorgefilipecosta Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

gziolo Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

jorgefilipecosta Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

gziolo Apr 9, 2018

Choose a reason for hiding this comment

gziolo commented Apr 5, 2018 •

edited

Loading

gziolo commented Apr 5, 2018 •

edited

Loading

paulwilde commented Apr 5, 2018 •

edited

Loading

jorgefilipecosta Apr 5, 2018 •

edited

Loading

gziolo Apr 5, 2018 •

edited

Loading

jorgefilipecosta Apr 5, 2018 •

edited

Loading